feat: add send and ack mutations Cloud Spanner Queues - #17728
feat: add send and ack mutations Cloud Spanner Queues #17728finn-the-coder wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces send and ack methods to both the async and sync Batch classes to support Cloud Spanner queues, along with corresponding unit and system tests. The feedback highlights that the unit tests incorrectly compare proto-plus message fields directly to raw Python values, which should be resolved by comparing protobuf messages using helper functions. Additionally, the system tests should be optimized to use the shared_instance fixture instead of creating and deleting new Spanner instances, and they should utilize pytest.skip() to properly report skipped tests when queues are not supported.
|
…include unit and system tests
- Use shared instance fixture - Update the shared instance to use ENTERPRISE_PLUS edition - Use pytest skip when Queues not supported
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for Cloud Spanner Queues by adding send and ack methods to the Batch class in both synchronous and asynchronous modules, along with corresponding unit and system tests. Feedback on these changes suggests removing an accidentally committed scratchpad file (test_spanner.py), switching the instance edition in system tests from ENTERPRISE_PLUS to ENTERPRISE to minimize testing costs, and safely retrieving the gRPC status code name using getattr in system tests to prevent potential AttributeErrors.
| from google.cloud.spanner_admin_instance_v1.types import spanner_instance_admin | ||
| request = spanner_instance_admin.UpdateInstanceRequest( | ||
| instance=spanner_instance_admin.Instance( | ||
| name="projects/my-project/instances/my-instance", | ||
| edition=spanner_instance_admin.Instance.Edition.ENTERPRISE, | ||
| ), | ||
| field_mask={"paths": ["edition"]}, | ||
| ) | ||
| print("SUCCESS") |
| display_name=shared_instance_id, | ||
| node_count=1, | ||
| labels=labels, | ||
| edition=spanner_instance_admin.Instance.Edition.ENTERPRISE_PLUS, |
There was a problem hiding this comment.
Using the ENTERPRISE_PLUS edition for system tests can significantly increase costs. Since Cloud Spanner Queues are also supported on the ENTERPRISE edition, consider using ENTERPRISE instead to minimize testing expenses.
| edition=spanner_instance_admin.Instance.Edition.ENTERPRISE_PLUS, | |
| edition=spanner_instance_admin.Instance.Edition.ENTERPRISE, |
| display_name=shared_instance_id, | ||
| node_count=1, | ||
| labels=labels, | ||
| edition=spanner_instance_admin.Instance.Edition.ENTERPRISE_PLUS, |
There was a problem hiding this comment.
Using the ENTERPRISE_PLUS edition for system tests can significantly increase costs. Since Cloud Spanner Queues are also supported on the ENTERPRISE edition, consider using ENTERPRISE instead to minimize testing expenses.
| edition=spanner_instance_admin.Instance.Edition.ENTERPRISE_PLUS, | |
| edition=spanner_instance_admin.Instance.Edition.ENTERPRISE, |
| except GoogleAPIError as e: | ||
| if ( | ||
| getattr(e, "code", None) == 501 | ||
| or ( | ||
| getattr(e, "grpc_status_code", None) | ||
| and e.grpc_status_code.name == "UNIMPLEMENTED" | ||
| ) | ||
| or "UNIMPLEMENTED" in str(e) | ||
| ): | ||
| pytest.skip(f"Queues are not implemented yet: {e}") | ||
| raise |
There was a problem hiding this comment.
Checking e.grpc_status_code.name directly can raise an AttributeError if grpc_status_code is not a gRPC StatusCode enum (e.g., if it is an integer or None in some environments). Using getattr to safely retrieve the status code name is more robust and avoids potential runtime errors during tests.
except GoogleAPIError as e:
grpc_status_name = getattr(getattr(e, "grpc_status_code", None), "name", None)
if (
getattr(e, "code", None) == 501
or grpc_status_name == "UNIMPLEMENTED"
or "UNIMPLEMENTED" in str(e)
):
pytest.skip(f"Queues are not implemented yet: {e}")
raise| except GoogleAPIError as e: | ||
| if ( | ||
| getattr(e, "code", None) == 501 | ||
| or getattr(e, "grpc_status_code", None) | ||
| and e.grpc_status_code.name == "UNIMPLEMENTED" | ||
| or "UNIMPLEMENTED" in str(e) | ||
| ): | ||
| pytest.skip(f"Queues are not implemented yet: {e}") | ||
| raise |
There was a problem hiding this comment.
Checking e.grpc_status_code.name directly can raise an AttributeError if grpc_status_code is not a gRPC StatusCode enum (e.g., if it is an integer or None in some environments). Using getattr to safely retrieve the status code name is more robust and avoids potential runtime errors during tests.
| except GoogleAPIError as e: | |
| if ( | |
| getattr(e, "code", None) == 501 | |
| or getattr(e, "grpc_status_code", None) | |
| and e.grpc_status_code.name == "UNIMPLEMENTED" | |
| or "UNIMPLEMENTED" in str(e) | |
| ): | |
| pytest.skip(f"Queues are not implemented yet: {e}") | |
| raise | |
| except GoogleAPIError as e: | |
| grpc_status_name = getattr(getattr(e, "grpc_status_code", None), "name", None) | |
| if ( | |
| getattr(e, "code", None) == 501 | |
| or grpc_status_name == "UNIMPLEMENTED" | |
| or "UNIMPLEMENTED" in str(e) | |
| ): | |
| pytest.skip(f"Queues are not implemented yet: {e}") | |
| raise |
There was a problem hiding this comment.
Is this added by mistake?
include unit and system tests
Fixes #17727